-
-
Notifications
You must be signed in to change notification settings - Fork 451
Improved error message for CMS page delete #4974
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- group by Mage_Core and Mage_Cms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves error messaging when attempting to delete or disable CMS pages that are used in configuration, and includes related improvements to the admin interface.
- Enhanced error messages to provide clearer context about which configurations are preventing page deletion/disabling
- Fixed sort order in admin config system.xml for CMS pages
- Added comprehensive test coverage for new functionality
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
app/code/core/Mage/Cms/Helper/Page.php | Added helper methods to generate user-friendly error messages with configuration labels and scope information |
app/code/core/Mage/Cms/Model/Resource/Page.php | Updated error message formatting and fixed store ID handling logic |
app/locale/en_US/Mage_Cms.csv | Improved error message templates for better readability |
app/code/core/Mage/Cms/etc/system.xml | Fixed sort order values for CMS page configuration options |
tests/unit/Mage/Cms/Helper/PageTest.php | Added unit tests for new helper methods |
tests/unit/Traits/DataProvider/Mage/Cms/CmsTrait.php | Added test data providers for the new functionality |
cypress/e2e/openmage/backend/cms/page.cy.js | Added end-to-end tests for page deletion and disabling scenarios |
cypress/support/openmage/config/paths.js | Added UI selectors for CMS page management |
cypress/support/openmage.js | Fixed save action behavior to prevent multiple clicks |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
@sreichel -Thank you for your work on this PR and for your kind words. However, I have some concerns and suggestions based on our previous implementations (PR #4239 and #4969) and on practical usability:
For the info message displaying where the page is used, I can drop the attempt to build a grammatically correct English sentence (method _joinWithCommaAnd in PR #4969). I believe these adjustments are crucial for a better user experience, especially in multi-store setups but this PR forces me to close my previous work, even though it could be easily adapted to incorporate these suggestions and best practices. |
All is done here, can you make suggestions to the code? public static function getScopeInfoFromConfigScope(string $scope, string $scopeId): string
{
return match ($scope) {
Mage_Adminhtml_Block_System_Config_Form::SCOPE_DEFAULT => Mage::helper('cms')->__('Default Config'),
Mage_Adminhtml_Block_System_Config_Form::SCOPE_WEBSITES => sprintf(
'%s "%s"',
Mage::helper('cms')->__('Website'),
Mage::app()->getWebsite($scopeId)->getName(),
),
Mage_Adminhtml_Block_System_Config_Form::SCOPE_STORES => sprintf(
'%s "%s"',
Mage::helper('cms')->__('Store View'),
Mage::app()->getStore($scopeId)->getName(),
),
};
}
This PR is ONLY for the message :P Changes in logic please in another PR. |
|
# Conflicts: # cypress/e2e/openmage/backend/catalog/categories.cy.js # cypress/e2e/openmage/backend/catalog/products.cy.js # cypress/e2e/openmage/backend/catalog/search.cy.js # cypress/e2e/openmage/backend/catalog/sitemap.cy.js # cypress/e2e/openmage/backend/catalog/urlrewrites.cy.js # cypress/e2e/openmage/backend/cms/block.cy.js # cypress/e2e/openmage/backend/cms/page.cy.js # cypress/e2e/openmage/backend/cms/widget.cy.js # cypress/e2e/openmage/backend/customer/customer.cy.js # cypress/e2e/openmage/backend/customer/group.cy.js # cypress/e2e/openmage/backend/customer/online.cy.js # cypress/e2e/openmage/backend/dashboard.cy.js # cypress/e2e/openmage/backend/newsletter/queue.cy.js # cypress/e2e/openmage/backend/newsletter/report.cy.js # cypress/e2e/openmage/backend/newsletter/subscriber.cy.js # cypress/e2e/openmage/backend/newsletter/templates.cy.js # cypress/e2e/openmage/backend/promo/catalog.cy.js # cypress/e2e/openmage/backend/promo/quote.cy.js # cypress/e2e/openmage/backend/sales/creditmemo.cy.js # cypress/e2e/openmage/backend/sales/invoice.cy.js # cypress/e2e/openmage/backend/sales/order.cy.js # cypress/e2e/openmage/backend/sales/shipment.cy.js # cypress/e2e/openmage/backend/sales/transactions.cy.js # cypress/e2e/openmage/backend/system/cache.cy.js # cypress/e2e/openmage/backend/system/config/catalog/configswatches.cy.js # cypress/e2e/openmage/backend/system/config/catalog/sitemap.cy.js # cypress/e2e/openmage/backend/system/config/customers/promo.cy.js # cypress/e2e/openmage/backend/system/design.cy.js # cypress/e2e/openmage/backend/system/emails.cy.js # cypress/e2e/openmage/backend/system/indexes.cy.js # cypress/e2e/openmage/backend/system/manage-currency.cy.js # cypress/e2e/openmage/backend/system/myacount.cy.js # cypress/e2e/openmage/backend/system/notifications.cy.js # cypress/e2e/openmage/backend/system/stores.cy.js # cypress/e2e/openmage/backend/system/variable.cy.js # cypress/e2e/openmage/frontend/customer/account/create.cy.js # cypress/e2e/openmage/frontend/newsletter-subscribe.cy.js # cypress/support/commands.js # cypress/support/e2e.js # cypress/support/openmage.js # cypress/support/openmage/backend/dashboard.js
…cms-error-message # Conflicts: # cypress/support/openmage/config/paths.js
# Conflicts: # skin/adminhtml/default/openmage/override.css.map
|
I’ve reviewed again the code and the error message in this PR. Unfortunately, the current error message is not clear enough for users:
For a better experience:
@copilot, please provide your feedback regarding the clarity, ordering, and helpfulness of the error message, and whether it should include direct links to the configuration section for easier navigation |
@addison74 your AI comments drive me bit crazy ...
See screenshot.
Ummm? Before that PR.
See 5ac59d3. Link to config section is there. |
Here is my reply: I understand that you didn't like my answer, but I can say the same thing about the fact that you made me close my contribution to CMS Pages and Default Pages, because "we have to do it step by step". We could have discussed the necessary changes, but you opened this PR without even asking my opinion. No problem I closed mine and I didn't put ethics into question. I offered you a visual comparison between your implementation and mine. My message is clear, I am pointing where the page is used, you just say Store View: French. Well, if you have 10 stores in French, which of them uses the page? If you consider your listing to be orderly, clear, then please convince the others and do not ask my opinion about this one. Here is the AI's reply: There is still ambiguity in the way locations are listed where the page is used as default. For example, if a page is used in a store view named "English", it is not clear which store view is meant, especially if there are several "French" store views across different websites and stores. The listing should provide full context, such as "Website: Main Shop > Store: Europe > Store View: French", not just "French". Otherwise, it is difficult for admins to know exactly where to make the change. Another important aspect is that listing the full path for each usage (Store View, Website View) can consume a lot of space in the error message area, especially when the page is used in multiple locations. This can make the message harder to read and less user-friendly, particularly for stores with many store views and websites. It would be helpful to optimize the way these locations are displayed—either by grouping, shortening > labels, or providing more compact formatting—so the message remains clear without overwhelming the admin interface. In conclusion, please consider revising the message to provide a logical, fully-qualified path for each usage, and order them (Default Config, Website, Store, Store View) for clarity. |
WTF? "we have to do it step by step" because you mixed up different thing over multiple PRs.
The current error message is bad. This PR targets this. ONLY this. It adds 60 line of code to improve it. I told you multipe time to check code in resource model, to not duplicate functionalty - but you ignored id. This is why i made the PR! Please manully test to it to make sure given information in error message are helpful. Feel free to work on every logic change, i'll do not. |
Description (*)
@addison74 thanks for working on all the CMS page related issue, but we should do it step by step ....
What are the issues we have?
This PR ...
Related Pull Requests
Manual testing scenarios (*)
Delete

Disable

Note
To run cypress ... use DDEV, install cypress plugin, run
ddev cypress-open -C .cypress.config.js